Skip to content

fix(sso): signal SSO denial to broker instead of forcing wp-login.php on anonymous /sso-grant#1321

Merged
superdav42 merged 1 commit into
mainfrom
fix/sso-grant-not-logged-in-redirect-to-login
May 29, 2026
Merged

fix(sso): signal SSO denial to broker instead of forcing wp-login.php on anonymous /sso-grant#1321
superdav42 merged 1 commit into
mainfrom
fix/sso-grant-not-logged-in-redirect-to-login

Conversation

@superdav42

@superdav42 superdav42 commented May 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes the /sso-grant 404 introduced by PR #1309 for anonymous visitors,
without forcing them through wp-login.php.

PR #1309 changed handle_broker()'s JSONP probe response from
{code: 0, message: 'Broker not attached'} (which triggered wu.sso_denied()
on the client and set the wu_sso_denied cookie) to
{verify: 'must-redirect', server: <main_url>} (which top-level navigates
to <main>/sso-grant?broker=...&token=...&checksum=...&return_url=<broker>).

That fix resolved a real bug — logged-in main-site users were getting
falsely denied on first hit — but it introduced a regression for
anonymous visitors on broker subsites:

  1. Anonymous visitor lands on a broker subsite (e.g. a free-checkout
    page, a landing page, a marketing page).
  2. sso.js runs the JSONP probe → main site responds must-redirect.
  3. Browser top-navigates to <main>/sso-grant?....
  4. handle_server() runs on the main site with no logged-in user and
    response_type = redirect — the not-logged-in non-JSONP branch.
  5. That branch fell through with the comment
    // Just let WordPress show the login page - don't exit, but
    /sso-grant is not wp-login.php, so WordPress rendered its
    404 template and Sidekick/page caches subsequently cached it,
    leaving the cross-domain SSO entry permanently broken for new
    anonymous visitors. Observed in production on the
    sdaiagent.com free-checkout flow today.

What this PR does

When handle_server() runs on /sso-grant for a not-logged-in user,
redirect back to the broker's /sso URL with sso_verify=invalid
appended.

This re-uses the existing denial-signalling pattern already implemented
in handle_broker() (the 'invalid' === $verify_code branch a few
hundred lines below): on receiving the signal, handle_broker() sets
wu_sso_denied=1 on the broker domain for 5 minutes and redirects to
the original return_url. sso.js (assets/js/sso.js:22) then sees
the cookie and skips the JSONP probe so the user browses anonymously
without further SSO disruption.

If the broker URL is missing (malformed grant request) we exit silently
rather than 404 or force login.

Why not redirect to wp-login.php?

An earlier revision of this PR redirected anonymous visitors to
wp_login_url($return_url). That was wrong for the same reason
falling through to the 404 template was wrong: the broker page the
visitor came from may be intentionally anonymous — free-checkout,
landing pages, public marketing pages. Forcing those visitors through
the WordPress login screen breaks the conversion flow they were on.

The denial-signal mechanism mirrors the explicit "anonymous, do
nothing" contract that existed before PR #1309 and re-uses the
infrastructure (wu_sso_denied cookie, sso.js skip path) that's
already in place. No new client-side code, no new cookie, no new
contract.

Regression test

tests/WP_Ultimo/SSO/SSO_Test.php
test_handle_server_source_signals_denial_when_not_logged_in_non_jsonp()

Source-pattern test (matching the convention already used in this
file because handle_server() calls exit) that asserts:

  1. The not-logged-in non-JSONP branch appends sso_verify=invalid to
    the broker URL via add_query_arg().
  2. The branch calls wp_safe_redirect($denial_url, 302, 'WP-Ultimo-SSO'); exit;.
  3. The branch does not call wp_safe_redirect(wp_login_url(...))
    (explicit guard catching the rejected earlier approach).
  4. The stale pre-fix(sso): return verify: must-redirect from unattached JSONP probe #1309 comment is gone.

Verification

Local dev (mygratis.site on the dev FrankenPHP instance, with this
branch checked out):

$ curl -sk -D - -o /dev/null \
    "https://mygratis.site/sso-grant?broker=TESTPROBE&token=test&checksum=test&return_url=https%3A%2F%2Ftesttenant.mygratis.site%2Fsso" \
    | grep -i -E "(HTTP|location|cache)"
HTTP/2 302
cache-control: no-cache, must-revalidate, max-age=0, no-store, private
location: https://testtenant.mygratis.site/sso?sso_verify=invalid
pragma: no-cache
x-ultimo-cache: prevent-caching

Pre-fix this returned HTTP 200 with the WordPress 404 template
(later cached as x-sidekick-cache: HIT on subsequent requests).

Test suite

$ vendor/bin/phpunit --filter test_handle_server_source_signals_denial \
    tests/WP_Ultimo/SSO/SSO_Test.php --no-coverage
OK (1 test, 6 assertions)

$ vendor/bin/phpunit --filter SSO_Test tests/WP_Ultimo/SSO/SSO_Test.php
OK (62 tests, 96 assertions)

$ vendor/bin/phpstan analyse inc/sso/class-sso.php --memory-limit=1G
[OK] No errors

$ vendor/bin/phpcs inc/sso/class-sso.php
(no new violations introduced)

Notes for reviewers

  • The cross-domain wp_safe_redirect() from the main site to a broker
    domain is permitted via the allow_network_redirect_hosts filter
    registered in inc/class-domain-mapping.php:61. Same mechanism
    handle_broker() already relies on for the existing sso_verify=invalid
    branch.
  • Pre-existing UM behaviour: wu_request() runs values through
    sanitize_text_field(), which strips %XX sequences. A nested
    return_url inside the broker URL will therefore lose its percent
    encoding. In practice the broker host stays intact (which is what
    matters for the denial cookie), and wp_safe_redirect() in
    handle_broker() falls back to home_url() for malformed
    return_url values. Fixing the broader URL-sanitisation behaviour
    is out of scope for this regression fix.

Refs #1309


aidevops.sh v3.20.5 plugin for OpenCode v1.15.12 with claude-opus-4-7 spent 1d 14h and 767,081 tokens on this with the user in an interactive session.

@coderabbitai

coderabbitai Bot commented May 29, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@superdav42, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 16 minutes and 15 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 78cf754d-c336-4a50-bd50-3775628e00e2

📥 Commits

Reviewing files that changed from the base of the PR and between be82600 and bed116a.

📒 Files selected for processing (2)
  • inc/sso/class-sso.php
  • tests/WP_Ultimo/SSO/SSO_Test.php
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sso-grant-not-logged-in-redirect-to-login

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown

🔨 Build Complete - Ready for Testing!

📦 Download Build Artifact (Recommended)

Download the zip build, upload to WordPress and test:

🌐 Test in WordPress Playground (Very Experimental)

Click the link below to instantly test this PR in your browser - no installation needed!
Playground support for multisite is very limitied, hopefully it will get better in the future.

🚀 Launch in Playground

Login credentials: admin / password

… on anonymous /sso-grant

Anonymous visitors hitting /sso-grant via the must-redirect flow from
PR #1309 were getting a WordPress 404, then a forced wp-login.php
redirect in the first version of this fix. Both are wrong for
intentionally-anonymous broker pages (free-checkout, landing pages,
marketing pages).

This change re-uses the existing denial-signalling pattern already
implemented in handle_broker (the 'invalid' === $verify_code branch):
when handle_server runs on /sso-grant for a not-logged-in user, redirect
back to the broker's /sso URL with sso_verify=invalid appended.

handle_broker on the broker domain recognises the signal, sets
wu_sso_denied=1 on the broker domain for 5 minutes, and redirects to
the original return_url. sso.js (assets/js/sso.js:22) then sees the
cookie and skips the JSONP probe so the user browses anonymously
without further SSO disruption.

Regression test updates accordingly (renamed
test_handle_server_source_redirects_to_login_when_not_logged_in_non_jsonp
to test_handle_server_source_signals_denial_when_not_logged_in_non_jsonp)
and adds an explicit assertDoesNotMatchRegularExpression guard that
catches any future attempt to call wp_safe_redirect(wp_login_url(...))
in that branch.

Refs #1309
@superdav42 superdav42 force-pushed the fix/sso-grant-not-logged-in-redirect-to-login branch from cdedc40 to bed116a Compare May 29, 2026 21:15
@superdav42 superdav42 changed the title fix(sso): redirect /sso-grant to wp-login.php when main-site user is not logged in fix(sso): signal SSO denial to broker instead of forcing wp-login.php on anonymous /sso-grant May 29, 2026
@github-actions

Copy link
Copy Markdown

🔨 Build Complete - Ready for Testing!

📦 Download Build Artifact (Recommended)

Download the zip build, upload to WordPress and test:

🌐 Test in WordPress Playground (Very Experimental)

Click the link below to instantly test this PR in your browser - no installation needed!
Playground support for multisite is very limitied, hopefully it will get better in the future.

🚀 Launch in Playground

Login credentials: admin / password

@superdav42 superdav42 merged commit f498c07 into main May 29, 2026
8 of 11 checks passed
@superdav42 superdav42 deleted the fix/sso-grant-not-logged-in-redirect-to-login branch May 29, 2026 21:47
@superdav42 superdav42 added the review-feedback-scanned Merged PR already scanned for quality feedback label May 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-feedback-scanned Merged PR already scanned for quality feedback

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant